expression: round function for int should use round half up rule #27403

feitian124 · 2021-08-19T15:19:40Z

What problem does this PR solve?

Issue Number: close #26993, close #27128

Problem Summary:

this pr aims to solve #26993 use suggestion from pr #27128. pr #27128 is deprecated and can be close by this pr too.

What's Changed:

remove old RoundFloat by using math.RoundToEven directly
add func RoundInt(i int64, dec int) int64 to round int
rename func Round(f float64, dec int) float64 to func RoundFloat(f float64, dec int) float64 to make name consistent with RoundInt

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Release note

None

ti-chi-bot · 2021-08-19T15:19:41Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

feitian124 · 2021-08-19T15:40:03Z

but i met some problem and had been blocked a few days, so i need help from you.

i fingerd out the test is blocked by the last commit int use RoundInt 08330da65de80c7fb656fb84a70de1be6ae3b7c8 . but this commit seems have no problem, i try debug and stepped one by one, TestRound in builtin_math_test.go seems all passed, and RoundInt seems all right.

if i revert it, the test will not block.
the message is:

[2021/08/19 22:47:02.838 +08:00] [INFO] [db.go:590] ["Memtable flushed"]
[2021/08/19 22:47:02.838 +08:00] [INFO] [db.go:594] ["Compaction finished"]
[2021/08/19 22:47:02.838 +08:00] [INFO] [db.go:613] ["BlobManager finished"]
[2021/08/19 22:47:02.838 +08:00] [INFO] [db.go:617] ["ResourceManager finished"]
[2021/08/19 22:47:02.838 +08:00] [INFO] [db.go:623] ["Waiting for closer"]

now i am blocked and don't know how to continue, please help.

feitian124 · 2021-08-19T15:46:01Z

could you help, or give some guide? thanks.
@ichn-hu @tisonkun

tisonkun · 2021-08-20T02:40:10Z

expression/integration_test.go

+	// MySQL 8 will report an error if round result overflows
+	rs, err = tk.Exec("select round(18446744073709551615, -19);")
+	c.Assert(err, NotNil)
+	terr = errors.Cause(err).(*terror.Error)
+	c.Assert(terr.Code(), Equals, errors.ErrCode(mysql.ErrDataOutOfRange))


[2021-08-19T15:56:19.448Z] FAIL: integration_test.go:413: testIntegrationSuite2.TestMathBuiltin [2021-08-19T15:56:19.448Z] [2021-08-19T15:56:19.448Z] integration_test.go:705: [2021-08-19T15:56:19.448Z] c.Assert(err, NotNil) [2021-08-19T15:56:19.448Z] ... value = nil

tests failed by above. I think you can take a look at "// for pow" part to see how to validate errors.

rs, err = tk.Exec("SELECT POW(0, -1);") c.Assert(err, IsNil) _, err = session.GetRows4Test(ctx, tk.Se, rs) c.Assert(err, NotNil) terr = errors.Cause(err).(*terror.Error) c.Assert(terr.Code(), Equals, errors.ErrCode(mysql.ErrDataOutOfRange)) c.Assert(rs.Close(), IsNil)

[2021-08-19T15:56:19.448Z] FAIL: integration_test.go:413: testIntegrationSuite2.TestMathBuiltin [2021-08-19T15:56:19.448Z] [2021-08-19T15:56:19.448Z] integration_test.go:705: [2021-08-19T15:56:19.448Z] c.Assert(err, NotNil) [2021-08-19T15:56:19.448Z] ... value = nil

tests failed by above. I think you can take a look at "// for pow" part to see how to validate errors.

thanks.
i learned validate error like above from the other tidb test files. i will investigate to learn more.

sorry and can't image the log clearly show why it fails but i did not notice..
I need be more careful..

wshwsh12 · 2021-08-20T03:23:13Z

types/helper.go

 	if math.IsNaN(result) {
 		return 0
 	}
 	return result
 }

+// RoundInt rounds the argument i to dec decimal places using "round half up" rule.
+// dec defaults to 0 if not specified. dec can be negative
+func RoundInt(i int64, dec int) int64 {


We should consider unsigned int range and overflow.

MySQL [test]> select round(9223372036854775808,-3); +-------------------------------+ | round(9223372036854775808,-3) | +-------------------------------+ | 9223372036854776000 | +-------------------------------+ 1 row in set (0.000 sec) MySQL [test]> MySQL [test]> select round(-9223372036854775808,-3); ERROR 1690 (22003): BIGINT value is out of range in 'round(-(9223372036854775808),-(3))'

thanks for suggestion, i will check

select round(1234567890123456789012345678901234567890, -30); # output: 1234567890000000000000000000000000000000

i found both mysql 8 and tidb v5.1.1have above result,
and 1234567890123456789012345678901234567890 is clearly much larger than math.MaxInt64,
so it cloud not be the param or return value of builtinRoundWithFracIntSig.evalInt,
i believe above case and unsigned int should have processed in some other place,
so i will not process these case in roundInt

I don't think so.
select round(1234567890123456789012345678901234567890, -30); This case is decimal round, not int.

We can create a table with bigint field and insert -9223372036854775808, then round it should throw the error.

Maybe we can try to port mysql's logic https://github.com/mysql/mysql-server/blob/beb865a960b9a8a16cf999c323e46c5b0c67f21f/sql/item_func.cc#L3413

feitian124 · 2021-08-23T14:09:30Z

/run-check_dev_2

feitian124 · 2021-08-23T14:32:26Z

/run-check_dev_2

feitian124 · 2021-08-23T14:58:57Z

i think i finished, my local test for package expression passed, not sure why ci check_dev_2 fail.
please help review, thanks
/cc @ichn-hu @wshwsh12 @XuHuaiyu

fuzhe1989 · 2021-08-23T15:20:07Z

cc @riteme since you have already finished this tough work in TiFlash.

riteme · 2021-08-24T01:50:06Z

/run-check_dev_2

riteme

How do you deal with unsigned integer round? I see there's two versions of abs for integers: one for int64 and another for uint64.

tidb/expression/builtin_math.go

Lines 67 to 70 in 22034c5

    
           _ builtinFunc = &builtinAbsRealSig{} 
        
           _ builtinFunc = &builtinAbsIntSig{} 
        
           _ builtinFunc = &builtinAbsUIntSig{} 
        
           _ builtinFunc = &builtinAbsDecSig{}

tisonkun · 2021-08-24T02:17:19Z

@feitian124 you can try to find the failure cause by downloading the log and match patterns listed here https://pingcap.github.io/tidb-dev-guide/get-started/write-and-run-unit-tests.html#how-to-find-the-failed-tests

feitian124 · 2021-08-24T03:46:50Z

How do you deal with unsigned integer round? I see there's two versions of abs for integers: one for int64 and another for uint64.

tidb/expression/builtin_math.go

Lines 67 to 70 in 22034c5

_ builtinFunc = &builtinAbsRealSig{}

_ builtinFunc = &builtinAbsIntSig{}

_ builtinFunc = &builtinAbsUIntSig{}

_ builtinFunc = &builtinAbsDecSig{}

current fix did not include change for unsigned integer , see above discuss with @wshwsh12

this fix is not perfect, but should have resolved the orignial issue which is caused by wrong round rule.

large number issue like unsigned integer and decimial which is out range of int64, it should be another problem out of range.

so i wonder if this pr can be merged first for 2 reason:

make pr not too large
the perfect fix need more knowledge and context, like how the sql is processed and dispatched, currently is a little diffcult to me.

feitian124 · 2021-08-24T03:50:33Z

@feitian124 you can try to find the failure cause by downloading the log and match patterns listed here https://pingcap.github.io/tidb-dev-guide/get-started/write-and-run-unit-tests.html#how-to-find-the-failed-tests

ok, thanks.
some times the log only have dozens of lines and failed

wshwsh12 · 2021-08-24T07:34:14Z

types/helper.go

+	}
+
+	shift := math.Pow10(-dec)
+	intPart := math.Round(float64(i) / shift)


Add a test case.

MySQL [test]> select round(24999999999999999,-16); +------------------------------+ | round(24999999999999999,-16) | +------------------------------+ | 20000000000000000 | +------------------------------+ 1 row in set (0.000 sec)

We should avoid float calc in int round.

added and failed, will fix later

tisonkun · 2021-08-25T10:51:12Z

/uncc

I may not be the reviewer to this PR. And it seems there is already reviewers work on this.

ti-chi-bot · 2021-09-21T20:32:50Z

@feitian124: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 19, 2021

github-actions bot added the component/expression label Aug 19, 2021

feitian124 mentioned this pull request Aug 19, 2021

expression: round function for int should use round half up rule #27128

Closed

4 tasks

ichn-hu mentioned this pull request Aug 19, 2021

Welcome to contribute #20804

Closed

tisonkun reviewed Aug 20, 2021

View reviewed changes

wshwsh12 reviewed Aug 20, 2021

View reviewed changes

feitian124 force-pushed the issue-26993-new branch 2 times, most recently from 8ca8620 to 09fc30f Compare August 23, 2021 03:37

feitian124 added 11 commits August 23, 2021 20:47

add tests

f724e87

add RoundInt

9d67a06

migrate types/helper_test.go to testify

774878f

remove old roundfloat function

09dce5c

add TestRoundInt

d9f7037

update comments

d0f31ee

rename Round to RoundFloat

53f0135

int use RoundInt

b294341

fix ci issue: fmt error

4c0e120

删掉可能导致block的测试用例

dd67250

use power instead of for loop

0f088e6

feitian124 force-pushed the issue-26993-new branch from 09fc30f to 0f088e6 Compare August 23, 2021 12:47

ti-chi-bot requested review from ichn-hu, wshwsh12 and XuHuaiyu August 23, 2021 14:58

feitian124 requested a review from tisonkun August 23, 2021 15:01

riteme reviewed Aug 24, 2021

View reviewed changes

Merge branch 'master' into issue-26993-new

39c1e99

wshwsh12 reviewed Aug 24, 2021

View reviewed changes

add test case for avoid float calc in int round

9593daa

ti-chi-bot removed the request for review from tisonkun August 25, 2021 10:51

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: round function for int should use round half up rule #27403

expression: round function for int should use round half up rule #27403

feitian124 commented Aug 19, 2021 •

edited

Loading

ti-chi-bot commented Aug 19, 2021

feitian124 commented Aug 19, 2021 •

edited

Loading

feitian124 commented Aug 19, 2021

tisonkun Aug 20, 2021

tisonkun Aug 20, 2021

feitian124 Aug 20, 2021 •

edited

Loading

wshwsh12 Aug 20, 2021

feitian124 Aug 20, 2021

feitian124 Aug 23, 2021 •

edited

Loading

wshwsh12 Aug 24, 2021

wshwsh12 Aug 24, 2021

feitian124 commented Aug 23, 2021

feitian124 commented Aug 23, 2021

feitian124 commented Aug 23, 2021 •

edited

Loading

fuzhe1989 commented Aug 23, 2021

riteme commented Aug 24, 2021

riteme left a comment

tisonkun commented Aug 24, 2021

feitian124 commented Aug 24, 2021 •

edited

Loading

feitian124 commented Aug 24, 2021

wshwsh12 Aug 24, 2021

feitian124 Aug 24, 2021

tisonkun commented Aug 25, 2021

ti-chi-bot commented Sep 21, 2021

	_ builtinFunc = &builtinAbsRealSig{}
	_ builtinFunc = &builtinAbsIntSig{}
	_ builtinFunc = &builtinAbsUIntSig{}
	_ builtinFunc = &builtinAbsDecSig{}

expression: round function for int should use round half up rule #27403

Are you sure you want to change the base?

expression: round function for int should use round half up rule #27403

Conversation

feitian124 commented Aug 19, 2021 • edited Loading

What problem does this PR solve?

Release note

ti-chi-bot commented Aug 19, 2021

feitian124 commented Aug 19, 2021 • edited Loading

feitian124 commented Aug 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feitian124 Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feitian124 Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feitian124 commented Aug 23, 2021

feitian124 commented Aug 23, 2021

feitian124 commented Aug 23, 2021 • edited Loading

fuzhe1989 commented Aug 23, 2021

riteme commented Aug 24, 2021

riteme left a comment

Choose a reason for hiding this comment

tisonkun commented Aug 24, 2021

feitian124 commented Aug 24, 2021 • edited Loading

feitian124 commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tisonkun commented Aug 25, 2021

ti-chi-bot commented Sep 21, 2021

feitian124 commented Aug 19, 2021 •

edited

Loading

feitian124 commented Aug 19, 2021 •

edited

Loading

feitian124 Aug 20, 2021 •

edited

Loading

feitian124 Aug 23, 2021 •

edited

Loading

feitian124 commented Aug 23, 2021 •

edited

Loading

feitian124 commented Aug 24, 2021 •

edited

Loading